Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(c-api) Implement wasm_exporttype_delete #1685

Merged
merged 4 commits into from
Oct 12, 2020
Merged

feat(c-api) Implement wasm_exporttype_delete #1685

merged 4 commits into from
Oct 12, 2020

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Oct 6, 2020

This PR implements the destructor for wasm_exporttype_t.

@Hywan Hywan requested a review from MarkMcCaskey October 6, 2020 20:21
@Hywan Hywan force-pushed the feat-c-api-exporttype-delete branch from 7443b30 to fdb0772 Compare October 6, 2020 20:22
MarkMcCaskey
MarkMcCaskey approved these changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! The one thing I'd want to confirm before shipping this is the ownership of wasm_exporttype_t, this looks like it has a legacy implementation so I'm a bit skeptical...

Yeah, exporttype_t owns both the things passed to it in new, so currently this implementation will leak memory because they're just stored as NonNull and never freed.

@Hywan
Copy link
Contributor Author

Hywan commented Oct 8, 2020 •
edited
Loading

wasm_exporttype_t is built with name and extern_type, both are NonNull. When wasm_exporttype_t is dropped, none of its fields are dropped, because NonNull does not free the pointer when it's dropped.

I believe it is up to the user to free the name and the extern_type in this case. wasm_exporttype_t just “owns pointers”, not the values.

Thoughts?

@Hywan
feat(c-api) Implement Drop for wasm_exporttype_t.
f24a34b 
`wasm_exporttype_t` has 2 fields: `name` and `extern_type`. Both are
of kind `NonNull`. When `wasm_exporttype_t` is dropped, nor `name` nor
`extern_type` are going to be dropped.

To avoid leaking data, this patch adds a new field: `owns_fields`:

* When `wasm_exporttype_t` is built from `wasm_exportype_new`, this
  field is set to `false` because `name` and `extern_type` are
  received by pointer, and its the responsibility of the caller to
  free them,

* When `wasm_exporttype_t` is built from the `From<&ExportType>`
  implementation, _we_ create `name` and `extern_type` to then leak
  them. In this case, it is safe to reconstruct proper `Box`es to
  finally drop them.
@Hywan
Copy link
Contributor Author

Hywan commented Oct 8, 2020 •
edited
Loading

Note: That's a little more trickier actually. It's likely that the wasm_exporttype_t will be created by wasm_module_exports. And thus, name and extern_type are created by our From<&ExportType> for wasm_exporttype_t implementation. Everything is put on on the stack, and no one owns it.

The solution would be to add an owns_fields: bool field to wasm_exporttype_t. If true, then we can Box::from_raw to construct a Box and then properly free the data. Otherwise, it's the responsability of the user to free it.

See my patch above: f24a34b

@Hywan
Copy link
Contributor Author

Hywan commented Oct 12, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 12, 2020

Build succeeded:

@bors bors bot merged commit bb9149b into wasmerio:master Oct 12, 2020
Hywan added a commit to Hywan/wasmer that referenced this pull request Oct 12, 2020
@Hywan
doc(changelog) Add wasmerio#1685.
4463294
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarkMcCaskey MarkMcCaskey MarkMcCaskey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Hywan @MarkMcCaskey